Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework PyAny::is_instance_of for performance #2881

Merged
merged 2 commits into from
May 19, 2023
Merged

Rework PyAny::is_instance_of for performance #2881

merged 2 commits into from
May 19, 2023

Conversation

davidhewitt
Copy link
Member

I was talking to @samuelcolvin about the fastest way to identify object types (relevant e.g. for pythonize and also pydantic-core) and noticed that PyAny::is_instance_of is quite unoptimised because it expands to an ffi call to PyObject_IsInstance.

This PR proposes PyAny::is_instance_of::<T>(obj) is changed to be equivalent to T::is_type_of(obj), plus add a sprinkling of inlining. We often have implementations such as PyDict_Check which can pretty much be optimised away to just checking a bit on the type object.

The accompanying benchmark to run through a bunch of object types is approx 40% faster after making this change.

For completeness, I've also added PyAny::is_exact_instance and PyAny::is_exact_instance_of, to pair with T::is_exact_type_of. (This could be split into a separate PR if preferred.)

birkenfeld
birkenfeld reviewed Jan 15, 2023
View reviewed changes
src/types/any.rs Outdated Show resolved Hide resolved
birkenfeld
birkenfeld reviewed Jan 15, 2023
View reviewed changes
src/types/any.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

Given the proximity to releasing 0.18 and that there's a couple of design choices / refinements to add here, I'm going to park this until after 0.18 is out.

@davidhewitt davidhewitt marked this pull request as draft January 15, 2023 12:44
@samuelcolvin
Copy link
Contributor

Idea looks great, thanks so much. But I agree, no massive hurry.

@davidhewitt davidhewitt mentioned this pull request May 15, 2023
Closed
7 tasks
@davidhewitt davidhewitt mentioned this pull request May 18, 2023
Merged
@davidhewitt
Copy link
Member Author

Rebased, changed PyAny::is_instance_of to return bool instead of PyResult<bool> and split out is_exact_instance[_of] into #3161.

The benchmark added in bench_any is about 30% faster for these changes compared to main. It's nice, though I'm not entirely sure it's worth the churn.

adamreichold
adamreichold approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@adamreichold adamreichold left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benchmark added in bench_any is about 30% faster for these changes compared to main. It's nice, though I'm not entirely sure it's worth the churn.

Considering the source of the churn is an improvement IMHO (dropping the Result layer) and that idiomatic Python API will often check types before committing to an implementation due the high cost of error/exception handling, I would say this is worth it.

Hot paths would probably still benefit from look-up tables as discussed in #3104 but this could give a good boost across the ecosystem where the LUT approach would be inappropriately heavy-weight.

@davidhewitt davidhewitt marked this pull request as ready for review May 19, 2023 06:34
@davidhewitt
Copy link
Member Author

Ok, that's a good argument in favour. Let's ship it!

bors r=adamreichold

@bors
Copy link
Contributor

bors bot commented May 19, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 3ec966d into PyO3:main May 19, 2023
@samuelcolvin samuelcolvin mentioned this pull request May 19, 2023
Closed
bors bot added a commit that referenced this pull request May 21, 2023
@bors @davidhewitt
Merge #3161
a3c4fd2 
3161: add PyAny::is_exact_instance and PyAny::is_exact_instance_of r=adamreichold a=davidhewitt

Split off from #2881. I'll rebase after that merges.

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@birkenfeld birkenfeld birkenfeld left review comments

@adamreichold adamreichold adamreichold approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidhewitt @samuelcolvin @birkenfeld @adamreichold